-
Notifications
You must be signed in to change notification settings - Fork 251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(node): add Node version to report #537
Conversation
tomlongridge
commented
May 14, 2019
- Added Node version to device.runtimeVersions.node to improve reporting
- Moved version info from metaData.device.
- Added Node version to device.runtimeVersions.node to improve reporting - Moved version info from metaData.device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks good and will work for error reports.
However, we also track sessions and the product spec states that we should add the device.runtimeVersions
field to the session payload as well. I believe it's possible to take a similar approach as we have already for this - @bengourley would have more context on this.
…nag-js into toml-node-version-reporting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial work you did on this is effective for both reports and sessions, as they both extract the device
property from the same client instance – the changes to session.js
are unnecessary. The change you've made replaces the exising client.device
property with the same data, essentially doing the same work twice. The device
property gets passed in to the session payload here.
If you remove those changes to the implementation, the assertions you added to the tests should still pass. Please leave them in – they're a good addition.
A couple of other suggestions for the test description and changelog entry.